Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activate bzlmod #1912

Merged
merged 18 commits into from
Jun 29, 2023
Merged

Activate bzlmod #1912

merged 18 commits into from
Jun 29, 2023

Conversation

ylecornec
Copy link
Member

@ylecornec ylecornec commented Jun 26, 2023

Depends on #1911

This PR contains the changes necessary to activate bzlmod in the CI.

  • A new rules_haskell_nix module is added, which is used to install nix toolchains with bzlmod. This way the main rules_haskell module does not have to depend on rules_nixpkgs (other than as a dev dependency).
    • In workspace mode the @rules_haskell//haskell:nixpkgs.bzl file is still available for backward compatibility (it is now a symlink to the file in rules_haskell_nix).
    • This @rules_haskell//haskell:nixpkgs.bzl symlink is also used by rules_haskell itselft for testing, but only as a dev dependency.
  • A local registry is used to access module from rules_nixpkgs until they are available on the central registry, as well as to access local versions of rules_haskell and rules_haskell_nix for testing.
  • The .bazelrc file is split into .bazelrc.common and .bazelrc.bzlmod. This way the various modules can share only the common part (they need to access the registry using different relative paths).
  • The new rules_haskell_dependencies extension contains dependencies which are always needed by rules_haskell (as opposed to the ones from non_module_dev_dependencies).
  • The new rules_haskell_asterius extension generates utility repository for asterius, and provide a custom tag to be able to use webpack from a pre-existing npm setup.

Changes to the runfiles library.

Since the name of repositories in the output base can change, the runfiles library must be able to resolve them.

As explained in this design document, this is done by parsing the _repo_mapping file which is now provided by bazel in the runfiles tree.

Since each repository has its own mapping, there is also some logic to recover the name of the current_repo, which is the repository making use of the runfiles library. A troublesome case is when a haskell library with runfiles is beeing used by a binary of another module. In this case, we cannot rely on the binary path to recover the current_repoof the library.

This is why a new createFromCurrentFile function was added to the runfiles library, to which we can pass the __FILE__ constant in order to recover the name of the module containing the library (similarly to what is mentioned in this section of the design document).

This requires enabling the preprocessor which has some limitation (such as being incompatible with multiline string), so the haskell_runfiles helper rule is provided in @rules_haskell//haskell:defs.bzl, which generates a small wrapper around the main runfiles library (to separate the usage of {-# LANGUAGE CPP #-} from the rest of the code).

Changes to the CI

  • Both WORKSPACE and bzlmod cases are now tested in the CI. This adds new jobs so the required status checks of github will need to be updated manually again.
  • On windows, we use a separate remote cache for bzlmod because the dependency analysis of C files is leaking absolute paths which are different in both cases.
  • The rules_haskell_nix module does not contains tests yet (it is tested by the rules_haskell_tests module in bzlmod mode), but at least buildifier should be added once more files are added to it (most notably the module extension to install nix toolchains).
  • The tutorial and example folders do not contain MODULE.bazel files yet so they are not tested with bzlmod.

@ylecornec ylecornec requested a review from avdv as a code owner June 26, 2023 17:35
@ylecornec ylecornec force-pushed the ylecornec/activate_bzlmod branch from 030bbdd to de4fc78 Compare June 26, 2023 17:47
Base automatically changed from ylecornec/bazel_6 to master June 27, 2023 06:39
@dpulls
Copy link

dpulls bot commented Jun 27, 2023

🎉 All dependencies have been resolved !

This commit add a custom registry for bzlmod support.
- It is used to access module from rules_nixpkgs until they are available on the central registry
- It is used to access local versions of `rules_haskell` and `rules_haskell_nix` modules for testing.
The legacy io_tweag_rules_nixpkgs repository is not a bazel module, so with bzlmod, we use the modules provided with rules_nixpkgs instead.
This module should be used with bzlmod to install nix based toolchains.
This way the main rules_haskell module does not depend on rules_nixpkgs.
- bzlmod is not setup yet in examples and tutorial directory
The docs folder has development dependencies that are not available from `rules_haskell_nix` and is not necessary for integration tests.
Various modules will need to reference the local registry with different relative paths.
For non development dependencies of rules_haskell which are not available as modules.
@ylecornec ylecornec force-pushed the ylecornec/activate_bzlmod branch from de4fc78 to c555631 Compare June 27, 2023 06:56
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Thank you!
This was a difficult task,

Just a couple minor comments.

@@ -1,414 +0,0 @@
"""Workspace rules (Nixpkgs)"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to watch out for: Symlinks in Zip archives are problematic, so turning this file into a symlink may make .zip bundles of rules_haskell unusable.

.github/workflows/workflow.yaml Show resolved Hide resolved
rules_haskell_tests/non_module_deps_2.bzl Show resolved Hide resolved
extensions/rules_haskell_dependencies.bzl Show resolved Hide resolved
haskell/asterius/extension.bzl Outdated Show resolved Hide resolved
import System.Info (os)

-- | Bazel repository mapping for bzlmod
-- See https://github.com/bazelbuild/proposals/blob/7c8da4a931d83db5c25abf85f6c486ad22d330e3/designs/2022-07-21-locating-runfiles-with-bzlmod.md
type RepoMapping = [((String, String), String)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps out of scope for this PR, but I wonder if we could overhaul this module to use some better data structures than [a] and String, like Map and perhaps even text (though I guess we want to stick to core package dependencies).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this file it seems that text is only a core package since ghc-8.4, but containers is there for all the versions mentioned in the file (which start at ghc-7.10.1).

I updated the library to use Data.Map.Strict in this commit (the lookupDir function makes use of Map.to_list but I think it's fine since we need to iterate over (key,value) pairs in this case).

tools/runfiles/src/Bazel/Runfiles.hs Outdated Show resolved Hide resolved
tools/runfiles/src/Bazel/Runfiles.hs Outdated Show resolved Hide resolved
haskell/defs.bzl Outdated Show resolved Hide resolved
haskell/defs.bzl Outdated
Comment on lines 798 to 810
native.genrule(
name = "{}_genrule".format(module_name),
outs = ["{}.hs".format(module_name)],
cmd = """
cat << EOF >> $@
{{-# LANGUAGE CPP #-}}
module {} (create, createFromProgramPath, rlocation) where
import qualified Bazel.Runfiles as RulesHaskellRunfiles
create = RulesHaskellRunfiles.createFromCurrentFile __FILE__
createFromProgramPath = RulesHaskellRunfiles.createFromProgramPathAndCurrentFile (Just __FILE__)
rlocation = RulesHaskellRunfiles.rlocation
EOF
""".format(module_name),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Maybe this could use Skylib's expand_template or write_file rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR to use expand_template 👍

@ylecornec ylecornec force-pushed the ylecornec/activate_bzlmod branch 2 times, most recently from e744032 to 9af561b Compare June 28, 2023 18:18
@ylecornec ylecornec force-pushed the ylecornec/activate_bzlmod branch 2 times, most recently from 0c9a5e2 to 8d445fa Compare June 28, 2023 19:18
@ylecornec ylecornec force-pushed the ylecornec/activate_bzlmod branch from 8d445fa to 8877ccc Compare June 28, 2023 20:26
@ylecornec
Copy link
Member Author

Thanks! I addressed the comments (except for the symlink one for which I have no good solution at the moment).

The required status checks of github need to be updated manually again to be able to merge, since new jobs were added to test bzlmod.

@aherrmann
Copy link
Member

The required status checks of github need to be updated manually again to be able to merge, since new jobs were added to test bzlmod.

done.

@aherrmann aherrmann added the merge-queue merge on green CI label Jun 29, 2023
@ylecornec ylecornec mentioned this pull request Jun 29, 2023
@ylecornec ylecornec merged commit cd23a0c into master Jun 29, 2023
@ylecornec ylecornec deleted the ylecornec/activate_bzlmod branch June 29, 2023 09:51
@mergify mergify bot removed the merge-queue merge on green CI label Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants